Skip to content

[py] Fix type annotation errors in service.py #16250

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 6 commits into from
Aug 27, 2025
Merged

[py] Fix type annotation errors in service.py #16250

merged 6 commits into from
Aug 27, 2025

Conversation

@NoStory-py
Copy link
Contributor

@NoStory-py NoStory-py commented Aug 24, 2025 •
edited
Loading

User description

Fixed the instance variable log_output by properly annotating it as int | IOBase | None. Removed unnecessary #type: ignore comments where mypy requires them.

🔗 Related Issues

Partially addresses #15697

💥 What does this PR do?

Fixes mypy type annotation errors in
selenium/webdriver/common/service.py:

  • Annotated self.log_output as int | IOBase | None.
  • Used cast() only where mypy requires it, removed unnecessary cast().
  • Removed unnecessary #type: ignore comments.

🔧 Implementation Notes

  • Annotated self.log_output as Optional[Union[int, IOBase]].
  • Alternative consideration was to simply use the annotation that the parameter log_output uses but that also includes str which the instance variable will never be str.
  • added cast() on self.log_output = log_output because parameter log_output is annotated with IO[Any] instead of IOBase.

💡 Additional Considerations

  • No tests are needed I already tested with mypy and ruff check
  • This is my first PR if more changes are needed please let me know.

🔄 Types of changes

  • Cleanup (formatting, renaming)

PR Type

Other

Description

  • Fix type annotation errors in service.py

  • Remove unnecessary type ignore comments

  • Add proper type annotation for log_output instance variable

  • Clean up cast() usage where not needed

Diagram Walkthrough

flowchart LR
  A["Type Annotation Issues"] --> B["Add log_output Type Annotation"]
  B --> C["Remove Unnecessary Casts"]
  C --> D["Remove Type Ignore Comments"]
  D --> E["Clean Python Code"]
Loading

File Walkthrough
Relevant files
Miscellaneous
service.py
Fix type annotations and remove unnecessary casts               

py/selenium/webdriver/common/service.py

  • Added explicit type annotation for log_output instance variable
  • Removed unnecessary cast() calls in conditional branches
  • Removed # type: ignore comments for Windows subprocess attributes
  • Added single cast() where mypy requires it for type compatibility
 +7/-6     

@CLAassistant
Copy link

CLAassistant commented Aug 24, 2025 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@selenium-ci selenium-ci added the C-py Python Bindings label Aug 24, 2025
@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Aug 24, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Type Narrowing

Ensure the declared type of self.log_output matches all assigned values and downstream usage. It is annotated as Optional[Union[int, IOBase]], but assignments include None, subprocess.DEVNULL (int), IOBase from open(), and cast(Union[int, IOBase], log_output). Validate that any code consuming self.log_output handles all three cases correctly, especially when None represents STDOUT.

 
self.log_output: Optional[Union[int, IOBase]]
if isinstance(log_output, str):
    self.log_output = cast(IOBase, open(log_output, "a+", encoding="utf-8"))
elif log_output == subprocess.STDOUT:
    self.log_output = None
elif log_output is None or log_output == subprocess.DEVNULL:
    self.log_output = subprocess.DEVNULL
else:
    self.log_output = cast(Union[int, IOBase],log_output)
Resource Handling

Opening a file when log_output is a string creates an IO handle; verify it is properly closed later (or managed contextually) to avoid file descriptor leaks, especially after changing types/casts that might affect lifecycle management assumptions.

 
if isinstance(log_output, str):
    self.log_output = cast(IOBase, open(log_output, "a+", encoding="utf-8"))
elif log_output == subprocess.STDOUT:

@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Aug 24, 2025 •
edited
Loading

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Handle log file open failures

Wrap file opening in a try/except to prevent crashes when the log file path is
invalid or not writable. Fall back to subprocess.DEVNULL or None to keep the
service start resilient.

py/selenium/webdriver/common/service.py [61-62]

 if isinstance(log_output, str):
-    self.log_output = cast(IOBase, open(log_output, "a+", encoding="utf-8"))
+    try:
+        self.log_output = cast(IOBase, open(log_output, "a+", encoding="utf-8"))
+    except OSError:
+        self.log_output = subprocess.DEVNULL
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: This suggestion correctly identifies a potential OSError when opening the log file, and the proposed try/except block makes the service initialization more resilient by preventing a crash.

Medium
General
✅ Ensure attribute is always initialized
Suggestion Impact:The commit applied the formatting fix by adding a space after the comma in the cast call. It did not add the default initialization to None.

code diff:

-            self.log_output = cast(Union[int, IOBase],log_output)
+            self.log_output = cast(Union[int, IOBase], log_output)

Initialize self.log_output with a default to avoid potential mypy "possibly
uninitialized" errors when new branches are added. Also fix minor formatting for
readability. This keeps the attribute always defined and improves
maintainability.

py/selenium/webdriver/common/service.py [60-68]

-self.log_output: Optional[Union[int, IOBase]]
+self.log_output: Optional[Union[int, IOBase]] = None
 if isinstance(log_output, str):
     self.log_output = cast(IOBase, open(log_output, "a+", encoding="utf-8"))
 elif log_output == subprocess.STDOUT:
     self.log_output = None
 elif log_output is None or log_output == subprocess.DEVNULL:
     self.log_output = subprocess.DEVNULL
 else:
-    self.log_output = cast(Union[int, IOBase],log_output)
+    self.log_output = cast(Union[int, IOBase], log_output)

[Suggestion processed]

Suggestion importance[1-10]: 3

__

Why: The suggestion to initialize self.log_output at declaration is good practice for robustness and readability, although the existing if/elif/else structure already ensures it is always assigned a value.

Low
  • Update

@NoStory-py
Fix type annotation errors in service.py
4f0434c 
Fixed the instance variable log_output by properly annotating
it as int | IOBase | None. Removed unnecessary #type: ignore
comments where mypy requires them.
@NoStory-py NoStory-py force-pushed the type-annotation-errors branch from 4262d10 to 4f0434c Compare August 24, 2025 16:39
navin772
navin772 requested changes Aug 25, 2025
View reviewed changes
@cgoldberg
Copy link
Member

cgoldberg commented Aug 25, 2025 •
edited
Loading

It's probably because I am using windows so mypy recognises the 'type' of this windows specific code on my end.

This is exactly the issue. We are using platform.system() for detecting Windows, but mypy doesn't respect that:

https://mypy.readthedocs.io/en/stable/common_issues.html#python-version-and-system-platform-checks

I wonder if we should change our platform detection across the entire codebase to use sys.platform instead? I thought platform.system() was the newer/better way, but it's causing issues here.

I don't really care if we leave the comments in this PR (we're going to get an error on one platform or the other either way). I'll open a new issue to investigate switching to sys.platform to avoid this.

@cgoldberg cgoldberg mentioned this pull request Aug 25, 2025
Closed
navin772
navin772 approved these changes Aug 26, 2025
View reviewed changes
Copy link
Member

@navin772 navin772 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! We can take care of windows error in the other issue created. Currently, mypy on CI runs on Ubuntu so it should not raise any type errors.

@navin772 navin772 merged commit 83cb4d6 into SeleniumHQ:trunk Aug 27, 2025
1 check passed
@NoStory-py NoStory-py deleted the type-annotation-errors branch September 3, 2025 06:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@navin772 navin772 navin772 approved these changes

Assignees

No one assigned

Labels

C-py Python Bindings Review effort 2/5

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@NoStory-py @CLAassistant @cgoldberg @navin772 @selenium-ci